Skip to content

[REVIEW] Add pytest-xdist to dev environment.yml - #6958

Merged
rapids-bot[bot] merged 5 commits into
NVIDIA:branch-0.18from
galipremsagar:parallel_pytest
Dec 16, 2020
Merged

[REVIEW] Add pytest-xdist to dev environment.yml#6958
rapids-bot[bot] merged 5 commits into
NVIDIA:branch-0.18from
galipremsagar:parallel_pytest

Conversation

@galipremsagar

@galipremsagar galipremsagar commented Dec 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Resolves: #6370

This PR enables the parallel execution of pytests of cudf, dask_cudf & custreamz in CI. The changes also include adding pytest-xdist to dev environments.

With these changes, here is the change in pytest execution times in CI:

module without pytest-xdist with pytest-xdist(n=6)
cudf 1 hr 14 min
dask_cudf 4 min 1 min
custreamz 6 min 2 min

Related Integration changes: rapidsai/integration#188

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team gpuCI Python Affects Python cuDF API. labels Dec 9, 2020
@galipremsagar galipremsagar self-assigned this Dec 9, 2020
@galipremsagar
galipremsagar requested a review from a team as a code owner December 9, 2020 21:53
@galipremsagar galipremsagar added feature request New feature or request non-breaking Non-breaking change labels Dec 9, 2020
@kkraus14

kkraus14 commented Dec 9, 2020

Copy link
Copy Markdown
Contributor

@galipremsagar I think we'd need to hook up pytest-xdist into CI in this PR as well: https://github.com/rapidsai/cudf/blob/branch-0.18/ci/gpu/build.sh#L202-L222

@galipremsagar

galipremsagar commented Dec 9, 2020

Copy link
Copy Markdown
Contributor Author

@galipremsagar I think we'd need to hook up pytest-xdist into CI in this PR as well: https://github.com/rapidsai/cudf/blob/branch-0.18/ci/gpu/build.sh#L202-L222

Thanks @kkraus14, I was under the impression integration repo was the place pytests were being triggered. Added it here aswell.

kkraus14
kkraus14 suggested changes Dec 9, 2020
View reviewed changes
Comment thread ci/gpu/build.sh
cd $WORKSPACE/python/cudf
gpuci_logger "Python py.test for cuDF"
py.test --cache-clear --basetemp=${WORKSPACE}/cudf-cuda-tmp --junitxml=${WORKSPACE}/junit-cudf.xml -v --cov-config=.coveragerc --cov=cudf --cov-report=xml:${WORKSPACE}/python/cudf/cudf-coverage.xml --cov-report term
py.test -n 6 --cache-clear --basetemp=${WORKSPACE}/cudf-cuda-tmp --junitxml=${WORKSPACE}/junit-cudf.xml -v --cov-config=.coveragerc --cov=cudf --cov-report=xml:${WORKSPACE}/python/cudf/cudf-coverage.xml --cov-report term

@kkraus14 kkraus14 Dec 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning behind 6 here? Should we set this programmatically based on something?

@galipremsagar galipremsagar Dec 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI test jobs have 8vCPUS, but only set PARALLEL_LEVEL to 4 to have a stable build. But for pytests we can easily use 6 to 7 cores. That was the reason I didn't set it programmatically here, and the reason to not use all 8 cores is to prevent choking of pytest execution.

@kkraus14 kkraus14 Dec 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rapidsai/ops any guidance / thoughts here? This looks good to go otherwise.

@ajschmidt8 ajschmidt8 Dec 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

Comment thread ci/gpu/build.sh
@galipremsagar galipremsagar added DO NOT MERGE Hold off on merging; see PR for details and removed DO NOT MERGE Hold off on merging; see PR for details labels Dec 10, 2020
@galipremsagar

galipremsagar commented Dec 14, 2020

Copy link
Copy Markdown
Contributor Author

rerun tests

1 similar comment
@galipremsagar

galipremsagar commented Dec 15, 2020

Copy link
Copy Markdown
Contributor Author

rerun tests

@codecov

codecov Bot commented Dec 15, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #6958 (d52b183) into branch-0.18 (f117b68) will increase coverage by 0.42%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #6958      +/-   ##
===============================================
+ Coverage        81.59%   82.02%   +0.42%     
===============================================
  Files               96       96              
  Lines            15905    16340     +435     
===============================================
+ Hits             12978    13403     +425     
- Misses            2927     2937      +10
Impacted Files Coverage Δ
python/cudf/cudf/core/dataframe.py 90.70% <0.00%> (-0.26%) ⬇️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_csv.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_json.py 100.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py 100.00% <0.00%> (ø)
python/cudf/cudf/utils/applyutils.py 98.74% <0.00%> (+0.02%) ⬆️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f117b68...d52b183. Read the comment docs.

@galipremsagar galipremsagar added DO NOT MERGE Hold off on merging; see PR for details 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer DO NOT MERGE Hold off on merging; see PR for details labels Dec 15, 2020
@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge 6 - Okay to Auto-Merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Dec 16, 2020
@rapids-bot
rapids-bot Bot merged commit 8c1f01e into NVIDIA:branch-0.18 Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes
+1 more reviewer
@kkraus14 kkraus14 kkraus14 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@galipremsagar galipremsagar

Labels

5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEA] Explore using pytest-xdist in CI

3 participants

@galipremsagar @kkraus14 @ajschmidt8